[cfg, recipe, tests] feat: wire DAPO overlong reward buffer on V1 omni loop - #502
OnePunchMonk wants to merge 12 commits into
Conversation
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new CPU test test_dapo_overlong_reward_on_cpu.py currently pulls gpt2 via AutoTokenizer.from_pretrained, which can introduce network/offline flakiness in CI and should be made self-contained.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Wires DAPO “overlong response” shaping configuration through verl_omni’s reward config so CLI overrides under reward.reward_kwargs.* reach verl’s registered DAPORewardManager, and adds smoke/CPU contract tests to ensure the overlong penalty path is actually exercised.
Changes:
- Add
reward.reward_kwargs(overlong buffer cfg +max_resp_len) toverl_omnireward config and propagate into generated trainer configs. - Add/adjust CPU tests to validate (a) algo config fields survive Hydra/dataclass conversion and (b) overlong shaping changes reward under
DAPORewardManager. - Add DAPO tiny-random V1 smoke script and DAPO example docs/launcher contract assertions.
File summaries
| File | Description |
|---|---|
| verl_omni/utils/config.py | Adds a warning guardrail when reward_manager=dapo is paired with GSPO loss mode. |
| verl_omni/trainer/config/reward/reward.yaml | Declares reward.reward_kwargs.* so reward-manager kwargs are not silently dropped. |
| verl_omni/trainer/config/algorithm.py | Updates OmniAlgoConfig to inherit upstream AlgoConfig for online/DAPO fields. |
| verl_omni/trainer/config/_generated_omni_trainer.yaml | Regenerates trainer config including reward.reward_kwargs. |
| verl_omni/trainer/config/_generated_omni_megatron_trainer.yaml | Regenerates trainer config including reward.reward_kwargs. |
| verl_omni/trainer/config/_generated_diffusion_veomni_trainer.yaml | Regenerates trainer config including reward.reward_kwargs. |
| verl_omni/trainer/config/_generated_diffusion_trainer.yaml | Regenerates trainer config including reward.reward_kwargs. |
| tests/workers/config/test_omni_config_on_cpu.py | Extends CPU config tests to assert inherited AlgoConfig fields survive Hydra conversion. |
| tests/utils/test_qwen3_omni_dapo_launcher_on_cpu.py | Adds contract tests for the AVQA launcher and the tiny-random DAPO smoke script, including overlong kwargs on the smoke. |
| tests/utils/test_dapo_overlong_reward_on_cpu.py | Adds a direct-unit CPU test ensuring overlong shaping changes the DAPO reward output. |
| tests/utils/test_config_on_cpu.py | Adds tests for the new GSPO+DAPO warning and the vanilla+DAPO non-warning case. |
| tests/special_e2e/run_dapo_qwen3_omni_thinker_lora_v1_smoke.sh | Adds a 2-GPU tiny-random DAPO V1 smoke that enables overlong shaping via reward.reward_kwargs.*. |
| examples/dapo_trainer/README.md | Documents the Thinker DAPO recipe and clarifies Phase 2 overlong shaping behavior and where it applies. |
| examples/dapo_trainer/qwen3_omni/run_qwen3_omni_thinker_dapo_lora_v1.sh | Adds the AVQA Thinker DAPO launcher (naive manager; dynamic sampling disabled). |
| docs/index.md | Adds the DAPO example page to the docs toctree. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Marking this reviewable once #456 (Phase 1) lands. This branch is stacked on Everything else in this PR is otherwise ready: CPU tests pass, GPU e2e smoke passes (see the PR description for the flashinfer environment detour — filed as #505/#506, unrelated to this PR's code), and the config-plumbing/reward-manager wiring is verified against upstream |
Assisted-by: AI tooling Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
Assisted-by: AI tooling Signed-off-by: Wenzhe Wang <w_wenzhe@163.com>
…i loop (verl-project#446 Phase 2) Phase 1 (verl-project#456) left the overlong reward buffer disabled everywhere; verl's registered DAPORewardManager already reads reward.reward_kwargs.overlong_buffer_cfg and reward.reward_kwargs.max_resp_len off the config, but verl_omni's reward.yaml never declared reward_kwargs, so any override there was silently dropped for the autogenerated trainer configs. - Add reward.reward_kwargs (overlong_buffer_cfg + max_resp_len) to reward.yaml and regenerate the four _generated_*.yaml trainer configs. - Enable overlong shaping in the DAPO tiny-random smoke test (reward.reward_manager.name=dapo path only; the naive-manager AVQA example is unaffected and documented as such). - Add a CPU test that drives DAPORewardManager directly and asserts the overlong penalty changes the reward on a truncated dummy response — the Phase 2 exit criterion from verl-project#446. - Split the shared launcher-contract test helper so the "no overlong" check only applies to the naive-manager example, and add overlong-specific assertions for the dapo-manager smoke test. This does not touch dynamic sampling (Phase 3) or the reward-loop metrics plumbing beyond what DAPORewardManager already logs in reward_extra_info. Draft: opened for early feedback while GPU LoRA validation on real Qwen3-Omni-30B-A3B-Instruct + gsm8k is still pending (see PR description). Co-authored-by: Claude Code
Co-authored-by: Claude Code
7177767 to
7dc3bd9
Compare
GPU smoke re-run after rebase onto #456's latest headRe-ran the Phase 2 GPU e2e smoke on Modal (2×A10G, CUDA 12.4.1 base + the same NUM_GPUS=2 TOTAL_TRAIN_STEPS=2 bash tests/special_e2e/run_dapo_qwen3_omni_thinker_lora_v1_smoke.shPer-sample DAPO reward scores again varied across the batch ( CPU tests also still pass on this head: python3 -m pytest tests/utils/test_dapo_overlong_reward_on_cpu.py tests/utils/test_qwen3_omni_dapo_launcher_on_cpu.py -q
# 3 passedMarking this ready for review now that it's rebased onto #456's latest and |
Signed-off-by: Avaya Aggarwal <119044997+OnePunchMonk@users.noreply.github.com>
| @@ -0,0 +1,118 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I know you plan to support dynamic sampling in #540. Why don't you move the smoke test to #540, so that you can enable both overlong reward buffer and dynamic sampling in the same test script?
Besides, to make this test take effect, you also need to add it to tests/gpu_smoke/run_gpu_smoke_omni_e2e.sh. If you have special pin version of third-party libraries, like flashinfer-python==0.6.16.post3, you may need to update .github/actions/gpu-smoke-prepare/action.yml.
|
@OnePunchMonk Thank you! Could you please provide the validation reward just like what is in #456 ? I am particularly interested in how the validation reward gets improved when overlong reward buffer is newly supported. @WenzheWang Please also give your valuable comments on this PR. |
WenzheWang
left a comment
There was a problem hiding this comment.
Thanks for wiring the overlong buffer configuration. I found one reproducible test failure and a gap in coverage of the configuration path introduced here:
-
Fix the shared launcher contract assertion. The new smoke test calls
_assert_dapo_without_dynamic_sampling_contract(smoke), but that helper still asserts thatoverlong_buffer_cfgis absent. The smoke intentionally enables it, so the new test fails before reaching its positive assertions. Runningpython -m pytest tests/utils/test_qwen3_omni_dapo_launcher_on_cpu.py -qagainst this head produced 1 failed, 1 passed. Please remove the no-overlong restriction from the shared helper and keep it in the naive-example test, which already checks it separately. -
Cover the actual configuration integration. The reward test constructs a handwritten
DictConfigand directly instantiatesDAPORewardManager, bypassing the changed YAML and registered factory. Please add a CPU regression test that composesomni_trainerwith non-defaultreward.reward_kwargs.*overrides, constructs throughload_reward_manager, and checks the forwarded settings and enabled/disabled penalty results. This would catch a missing or misnested YAML field that the current test cannot detect.
The existing tokenizer/offline-test comment also remains applicable. I will avoid duplicating the ongoing smoke-placement and README discussions.
…se/issue refs
- Replace AutoTokenizer.from_pretrained("gpt2") with an in-memory
WordLevel tokenizer so the overlong-reward CPU test needs no network
access (Copilot, wtomin).
- Rewrite the overlong reward buffer section of examples/dapo_trainer/README.md
as plain usage instructions instead of phase/issue references (wtomin).
…oth scripts The PR description says the "no overlong config" assertion was split so it only applies to the naive-manager example, but the shared helper _assert_dapo_without_dynamic_sampling_contract() still carried a blanket "overlong_buffer_cfg not in script" check, so it failed for the dapo smoke script that legitimately enables it. The phase-1 naive-example test already has its own standalone assertion for this, so drop the duplicate from the shared helper.
Summary
Implements Phase 2 of #446 (Qwen3-Omni Thinker DAPO): wires the overlong
response-length penalty into the V1 omni reward path.
Stacked on #456 (Phase 1) — not yet merged. This branch is rebased onto
wenzhe/qwen3-omni-thinker-dapoatbc0bee6(current #456 head), so the diffagainst
mainincludes #456's commits. Please review only the two commits ontop (
feat: wire DAPO overlong reward buffer on V1 omni loop (#446 Phase 2),chore: trim redundant comments) until #456 lands; I'll rebase and shrink thediff once it merges.
2026-09-04 update: rebased onto #456's latest head (it had moved since
this PR was opened — the Phase-1 GPU smoke script was removed there per
review feedback, since the existing GSPO smoke covers the same pipeline).
Phase 2 restores that smoke script under its own scope, since it's this PR's
own e2e coverage for the overlong buffer, not a reintroduction of the removed
Phase-1 smoke. CPU tests pass post-rebase; the GPU e2e run below predates the
rebase and should be re-run before merge — I don't have further Modal/GPU
access in this pass.
What's actually wired
verl's registereddaporeward manager(
verl.experimental.reward_loop.reward_manager.dapo.DAPORewardManager) alreadyreads
config.reward.reward_kwargs.overlong_buffer_cfgandconfig.reward.reward_kwargs.max_resp_len— butverl_omni'sreward.yamlnever declared a
reward_kwargskey, so any++reward.reward_kwargs.overlong_buffer_cfg.enable=trueoverride on the CLIwas silently swallowed rather than reaching the manager. This PR:
reward.reward_kwargs(overlong_buffer_cfg.{enable,len,penalty_factor,log},max_resp_len) toverl_omni/trainer/config/reward/reward.yaml, disabled bydefault, and regenerates the four
_generated_*.yamltrainer configs viascripts/generate_trainer_config.sh.(
tests/special_e2e/run_dapo_qwen3_omni_thinker_lora_v1_smoke.sh), sizedagainst its own
data.max_response_length=512. The naive-manager AVQAexample (
examples/dapo_trainer/qwen3_omni/run_qwen3_omni_thinker_dapo_lora_v1.sh)is untouched and documented as such — overlong shaping is a no-op under
reward.reward_manager.name=naive.tests/utils/test_dapo_overlong_reward_on_cpu.py, which drivesDAPORewardManagerdirectly (no Hydra/trainer bring-up) and asserts theoverlong penalty actually changes the reward on a truncated dummy response
— this is Phase 2's exit criterion from the issue.
tests/utils/test_qwen3_omni_dapo_launcher_on_cpu.pyso "no overlongconfig" only applies to the naive-manager example, and adds
overlong-specific assertions for the dapo-manager smoke test.
Not in scope: dynamic sampling (Phase 3), and any reward-loop metrics
plumbing beyond what
DAPORewardManageralready logs intoreward_extra_info(
overlong,overlong_reward).Test plan
CPU (all green locally):
GPU e2e (2-GPU tiny-random smoke, exercises the new
reward.reward_kwargs.*overrides end-to-end through the real V1 trainer loop):
Ran this on a Modal 2×A10G box against an improvised environment (CUDA
12.4.1 base + the exact pins from
.github/actions/gpu-smoke-prepare/action.yml),since I don't have access to CI's private
cu130registry image.First two attempts hit environment issues unrelated to this PR: a missing
apt package, then a genuine upstream bug in
flashinfer-python==0.6.16.post3(pulled in transitively via
vllm==0.27.0, no floor pinned anywhere in thisrepo) that crashes any
TP>=2GPU run at import time. Filed as #505, fixedin #506 (floors
flashinfer-python>=0.6.16.post4in thegpuextra) — seethose for the full root-cause writeup.
With
flashinfer-python==0.6.16.post4pinned (an environment-only override,nothing in this repo changed), the smoke test passes:
Per-sample DAPO reward scores varied across the batch (
-1.0,-0.3438,-0.0625), consistent with the overlong-buffer penalty grading responsesapproaching/at
max_resp_len=512(bufferlen=128, so penalty onset at~384 tokens, full -1.0 at 512).
One caveat I'm not glossing over: I could not find an explicit
step:N - reward/overlong...console-logger line in the captured output —grepped the full stdout+stderr for
step:and got zero matches for anymetric, not just overlong, so this looks like a console-logging/capture gap
in this V1 path rather than evidence the wiring is broken (the reward
computation itself clearly executed through the DAPO path, per the reward
scores above). Worth a maintainer's eyes on whether metrics are expected to
print to console here or only to the configured tracker backend.
AI usage disclosure
Claude Code was used to trace the config-plumbing gap, implement this change,
and write the tests described above. A human submitter (onepunchmonk) has
reviewed every changed line and will review the Modal GPU run before taking
this out of draft.